Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use the pre-convention type mapping configuration in query #25509

Merged
merged 3 commits into from
Aug 16, 2021

Conversation

AndriySvyryd
Copy link
Member

Part of #25084

@AndriySvyryd AndriySvyryd requested a review from a team August 13, 2021 16:43
@AndriySvyryd
Copy link
Member Author

@smitpatel @ajcvickers This change propagates the model to the query services by making them scoped. This worked better than I expected and won't break providers as long as they use the correct service registration pattern (our NTS plugins didn't).

However now I'm wondering whether this is what the users would expect to happen. They configure the mapping for Properties(), but we apply it for values that aren't mapped to any property, like constants and function returns. Perhaps we should introduce a separate method on ModelConfigurationBuilder just for this, something like Scalars()

@AndriySvyryd
Copy link
Member Author

Also Properties() lets one to configure properties using the base type or generic definition, e.g. Properties<MyWrappedType<>>(). For scalars this is probably less useful and might be unexpected. It also adds complexity to type mapping finding code.

I propose that Scalars<T>() will only apply to T and T? if it's a value type and we will throw for Scalars<T?>() or if T is non-instantiable.

@AndriySvyryd AndriySvyryd changed the base branch from main to Issue25468 August 14, 2021 00:04
@ajcvickers
Copy link
Member

I agree with the concerns over Properties, but I worry that figuring out when to use Properties and when to use Scalars will be hard. Even figuring what is and isn't a "scalar" can be hard. But I can also see that a single set of semantics (i.e. one method, however it is named) doesn't work very well. I wonder if we should punt on this for 6.0 and wait for feedback?

@AndriySvyryd AndriySvyryd marked this pull request as ready for review August 14, 2021 18:53
@AndriySvyryd
Copy link
Member Author

I agree with the concerns over Properties, but I worry that figuring out when to use Properties and when to use Scalars will be hard. Even figuring what is and isn't a "scalar" can be hard. But I can also see that a single set of semantics (i.e. one method, however it is named) doesn't work very well.

This will probably require good documentation or perhaps just start with a warning that Scalars is not commonly used. In testing I found that it is rarely needed as we do a good job of inferring mappings, however in places where it is needed there are no workarounds except writing a type mapper plugin.

I wonder if we should punt on this for 6.0 and wait for feedback?

Or we can ship it in RC1 and wait for feedback.

@AndriySvyryd AndriySvyryd changed the base branch from Issue25468 to main August 15, 2021 18:10
@ajcvickers
Copy link
Member

Okay with me to ship in RC1.

@AndriySvyryd AndriySvyryd merged commit 48eaf85 into main Aug 16, 2021
@AndriySvyryd AndriySvyryd deleted the Issue25084_scoped branch August 16, 2021 22:52
@roji
Copy link
Member

roji commented Aug 23, 2021

This change propagates the model to the query services by making them scoped. This worked better than I expected and won't break providers as long as they use the correct service registration pattern (our NTS plugins didn't).

Just spent a few hours tracking down some seemingly unrelated test failures to this...

@AndriySvyryd
Copy link
Member Author

Just spent a few hours tracking down some seemingly unrelated test failures to this...

Any suggestions for other providers? Should we emphasize that TryAddProviderSpecificServices should only be used for provider-specific services?

@roji
Copy link
Member

roji commented Aug 23, 2021

Yeah... In theory would it be possible to actually check if services registered via TryAddProviderSpecificServices are actually not provider-specific, and throw an informative message? Though if that's complicated it may not be worth it...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants